Merge fit() and fit_dataset(). Replace summary() with transform(). Add fit_transform().#112
Merge fit() and fit_dataset(). Replace summary() with transform(). Add fit_transform().#112JulioAPeraza wants to merge 3 commits intoneurostuff:masterfrom
fit() and fit_dataset(). Replace summary() with transform(). Add fit_transform().#112Conversation
JulioAPeraza
changed the title
fit() and fit_dataset(). Replace summary() with transform(). Add fit_transform().fit() and fit_dataset(). Replace summary() with transform(). Add fit_transform().
tsalo
left a comment
tsalo
left a comment
There was a problem hiding this comment.
I totally forgot about this PR. Sorry about that!
| _dataset_attr_map = {"z": "y", "w": "v"} | ||
|
|
||
| def fit(self, z, w=None): | ||
| def _fit(self, z, w=None): |
There was a problem hiding this comment.
Yeah, this one was tricky because fit() alone only works with datasets while _fit() is fitted to arrays. This was a solution that worked, but I'm sure there is a more elegant one that can make that distinction clearer.
Would you suggest a different approach? Thanks!
There was a problem hiding this comment.
To be honest, I don't remember if I had considered how fitting to arrays would work with the proposed changes... I wish I'd made a note about it in #103.
We don't want users to call a private method, so I think the options are to either make the array-fitting method a different public method (e.g., fit_array) or to support arrays in fit. WDYT?
There was a problem hiding this comment.
I think if fit() alone cannot distinguish between an array or dataset we should keep fit() only for arrays for the sake of scikit-learn styling, and fit_dataset() for datasets.
Does that mean we need to add fit_dataset_transform() in addition to fit_transform()?
There was a problem hiding this comment.
I think we want the Dataset to be the default (unless I'm misremembering), so I think fit_array and fit would be better than fit and fit_dataset. It's a difficult thing though, so maybe we should bring in the rest of the neurostore team on this?
2 participants
Closes #103.
Changes proposed in this pull request:
fit()andfit_dataset()in BaseEstimator.fit_transform()in BaseEstimator, to fit and transform data in one step.fit()to_fit()for all estimators.test_estimator_fit_transform.